Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix/180] Fix offset for vector instructions that operate in private memory space #181

Merged
merged 5 commits into from
Mar 14, 2022
Merged

[fix/180] Fix offset for vector instructions that operate in private memory space #181

merged 5 commits into from
Mar 14, 2022

Conversation

stratika
Copy link
Collaborator

@stratika stratika commented Mar 11, 2022 •
edited
Loading

This PR addresses the issue described in #180. The problem is that the compiler was generating the same offset for accessing all the elements within a vector.
For example, if a vector of type VectorFloat2 has four Float2 elements, this should result in the emission of four consecutive vload and vstore operations (one per element). In this case the compiler would overwrite and load the data of the first element.

This PR resolves this issue by providing a method to calculate the offset and use it when the memory space corresponds to private memory.

In a nutshell, it includes:

  • fix of the problem in OCLArithmeticTool class.
  • addition of unit-tests to cover all supported vector types
  • addition of getLength method for VectorFloat, VectorInt, VectorDouble types, in order to be consistent with other vector types.

Backend/s tested

Mark the backends affected by this PR.

  • OpenCL
  • PTX
  • SPIRV

OS tested

Mark the OS where this PR is tested.

  • Linux
  • OSx
  • Windows

Did you check on FPGAs?

If it is applicable, check your changes on FPGAs.

  • Yes
  • No

How to test the new patch?

tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestDoubles#privateVectorDouble2
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestDoubles#privateVectorDouble4
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestDoubles#privateVectorDouble8
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestInts#privateVectorInt2
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestInts#privateVectorInt4
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestInts#privateVectorInt8
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestFloats#privateVectorFloat2
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestFloats#privateVectorFloat4
tornado-test.py --verbose --printKernel uk.ac.manchester.tornado.unittests.vectortypes.TestFloats#privateVectorFloat8

This functionality is currently not supported in SPIR-V. We will open a separate PR for this.

@stratika stratika self-assigned this Mar 11, 2022
@stratika stratika added enhancement New feature or request OpenCL labels Mar 11, 2022
@stratika stratika linked an issue Mar 11, 2022 that may be closed by this pull request
Instantiating Vector types (e.g. VectorFloat4) inside a Tornado task produces wrong results #180
Closed
jjfumero
jjfumero requested changes Mar 11, 2022
View reviewed changes
...pencl/src/main/java/uk/ac/manchester/tornado/drivers/opencl/graal/lir/OCLArithmeticTool.java Outdated
if (memoryAccess.getIndex() instanceof ConstantValue) {
ConstantValue constantValue = (ConstantValue) memoryAccess.getIndex();
int parsedIntegerIndex = Integer.parseInt(constantValue.getConstant().toValueString());
int s = parsedIntegerIndex / oclKind.getVectorLength();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename s to index

unittests/src/main/java/uk/ac/manchester/tornado/unittests/vectortypes/TestDoubles.java Outdated
testPrivateVectorDouble8(sequentialOutput);

for (int i = 0; i < size; i++) {
assertEquals(sequentialOutput.get(i).getS0(), tornadoOutput.get(i).getS0(), 0.001);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a constant for the delta value 0.001

@stratika
Copy link
Collaborator Author

thanks @jjfumero, I made the changes. I also removed the delta from the Integer unit-tests.

@jjfumero jjfumero merged commit f05c68d into beehive-lab:develop Mar 14, 2022
@stratika stratika deleted the fix/180 branch March 22, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjfumero jjfumero jjfumero approved these changes

@gigiblender gigiblender gigiblender approved these changes

Assignees

@stratika stratika

Labels
enhancement New feature or request OpenCL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Instantiating Vector types (e.g. VectorFloat4) inside a Tornado task produces wrong results
3 participants
@stratika @jjfumero @gigiblender